feat: Add undoable message delete with UI-layer snackbar actions and cleaner store boundaries#901
feat: Add undoable message delete with UI-layer snackbar actions and cleaner store boundaries#901AlliotTech wants to merge 6 commits intogotify:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #901 +/- ##
=======================================
Coverage 79.15% 79.15%
=======================================
Files 56 56
Lines 2226 2226
=======================================
Hits 1762 1762
Misses 360 360
Partials 104 104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eternal-flame-AD
left a comment
eternal-flame-AD
left a comment
There was a problem hiding this comment.
Thanks, please see comments
eternal-flame-AD
left a comment
eternal-flame-AD
left a comment
There was a problem hiding this comment.
Are there any specific difficulties making the delete all button also undoable ? A cursory look it seems like it's just deleting with a magic number -1.
| appIndex: false | number | ||
| ) => { | ||
| if (allIndex !== false && this.exists(AllMessages)) { | ||
| this.state[AllMessages].messages.splice(allIndex, 0, message); |
There was a problem hiding this comment.
Good to have: Don't make this assumption here. there is an edge case where if you delete the message, send a new one in and then try to undo the order can get swapped.
No need if it's too complicated
| if (options?.keepalive) { | ||
| const response = await authFetch(url, { | ||
| method: 'DELETE', | ||
| keepalive: true, | ||
| credentials: 'include', | ||
| }); | ||
| if (!response.ok) { | ||
| throw new Error('Failed to delete message'); | ||
| } | ||
| return; | ||
| } | ||
| this.snack('Message deleted'); | ||
| await axios.delete(url); |
There was a problem hiding this comment.
| if (options?.keepalive) { | |
| const response = await authFetch(url, { | |
| method: 'DELETE', | |
| keepalive: true, | |
| credentials: 'include', | |
| }); | |
| if (!response.ok) { | |
| throw new Error('Failed to delete message'); | |
| } | |
| return; | |
| } | |
| this.snack('Message deleted'); | |
| await axios.delete(url); | |
| await axios.delete(url, { | |
| fetchOptions: { | |
| keepalive: options?.keepalive, | |
| credentials: 'include', | |
| }, | |
| }); |
There was a problem hiding this comment.
it seems like Axios is still using the default XHR adapter (no adapter configured), so fetchOptions is ignored and keepalive won’t take effect. 👀
There was a problem hiding this comment.
Seems to work with
await axios.delete(config.get('url') + 'message/' + message.id, {
adapter: 'fetch',
fetchOptions: {keepalive: true},
});
| @@ -0,0 +1,139 @@ | |||
| import Button from '@mui/material/Button'; | |||
There was a problem hiding this comment.
I feel like the current implementation is too complex, the storing of indexes seems error prone, as the index changes with each delete and each new message.
I think it should be possible to implement this functionality without modifying the message state for pending deletions and only modify it when there is an actual delete.
I've an idea in mind and will draft something today or later this week.
3 participants
accidental deletes within a short window.
restore) into the Messages page to keep stores UI-agnostic and
aligned with existing architecture.
related issue: #791